adding Java SDK client example and scripts to run it on openshift #46

oburstein-hub · 2020-02-05T13:59:11Z

What does this PR do? Is there any background context you want to provide?

This PR provides Conjur Java SDK Client Sample application along with scripts for building a docker image for it and then installing the above on existing OpenShift System.
In addition this PR provides script which installs Conjur OSS and Conjur CLI with some basic configuration (running it is a precondition for a steps above).
The script installing Conjur OSS and CLI can be usefull for any basic automatic installation of Conjur on OpenShift

What ticket does this PR close?

Connects to [relevant GitHub issues, eg #76]
NA

Where should the reviewer start? How should this functionality be validated?

The reviewer should start at Java Client source code and pom.xml, then contirue to the build.sh and then to installer.sh and java-client-installer.sh

Screenshots (if appropriate)

Links to open issues for related documentation (in READMEs, docs, etc)

Connects to [relevant GitHub issues, eg cyberark/conjur-docs#76]
NA

jvanderhoof · 2020-02-06T15:42:52Z

demos/java-api-client/build.sh

+}
+
+validate_app git
+validate_app mvn


How about we use the Maven container to compile: https://hub.docker.com/_/maven. This reduces the dependencies a user needs on their machine.

does it mean to run script from docker ?

I have thought about it a little bit more - it is a very good idea - but if I will do it it will mean that after running my script in docker the JAVA API SDK will be installed only in maven repository inside that docker container, and thus will not be usable for another developers. The intention was to solve the problem JFC was facing with - and it means to have Java API SDK be installed in local maven repo after running the script.

This project is intended to be demo tool, not a build tool. Could we move this build specific functionality into the cyberark/conjur-api-java project instead?

The goal for this project is to minimize dependencies required by the user, hence the push to use Docker images. It's not intended to replicate a development environment. Each project should enable development within it's scope.

conjur-java-api build specific functionality should be added to conjur-java-api by US Team (please inform me if it your team going to do it) - and this build should be a part of pipeline and put conjur-java-api FAT JAR to global maven repository.
As soon as it will be done I will remove conjur-java-api building part from my build script - and I will leave there only build of Java Conjur API client because it is an example for Demos

Regarding adding docker to the project: I was told that there is explicit requirement from customers not to add docker because many of them are not going to use docker in their developments and thus example using docker is less relevant to them

Once again. This is not a developer project. This is a project for to demonstrate scenarios. Development tools should be part of the original project (cyberark/conjur-api-java in this case).

jvanderhoof · 2020-02-06T15:43:59Z

demos/java-api-client/build.sh

@@ -0,0 +1,68 @@
+#!/bin/bash


Mind adding an -x flag? Scripts like this should exit if there are any issues during the run to make it obvious there is a problem.

it should not stop on any error. For example if we have some error in prune images - it does not have to be a problem. Thus I have added exit conditions related to specific commands

If you have commands that you don't mind failing, I think you should prevent an error message from appearing or add a message that explains that it's ok. Otherwise, the user won't have high confidence that the run was successful. With oc commands, you can add the flag --ignore-not-found to prevent these errors.

jvanderhoof · 2020-02-06T15:49:41Z

demos/java-api-client/build.sh

+echo "$COMMAND"
+suffix="/build.sh";
+HOME_DIR=${COMMAND%$suffix};
+pushd $HOME_DIR


I'd never heard of pushd. Neat!

jvanderhoof · 2020-02-06T16:00:22Z

demos/open-shift-install/installer.sh

+  validate_app docker-compose
+  validate_app awk
+  validate_app openssl
+  validate_app keytool


It feels like Docker (and Docker-Compose) are standard. What do you think about adding a container with Helm, OC, awk, openssl, and keytool so that a user doesn't need to have all these locally?

It is a good idea, but I would prefer to do it in a separate phase and to keep both ways of activation available for customers. What do you think ?

This project is intended to empower non-developers run some of these more complex workflows easily. If we put additional requirements of our uses, we limit our audience.

agree, but the tradeoff is between putting additional requirements and mandating docker usage as part of example

jvanderhoof · 2020-02-06T16:01:15Z

demos/open-shift-install/installer.sh

+    echo "Create certificate"
+    oc exec -it "$CONJUR_CLI_POD" ./conjur_scripts/cert_script.sh $ACCOUNT_NAME $AUTHENTICATOR
+    oc exec -it "$CONJUR_CLI_POD" cat /root/conjur-$ACCOUNT_NAME.pem > conjur-cert.pem
+  }  &> /dev/null


Don't we want errors visible to the user?

improved error visibility for user

jvanderhoof · 2020-02-07T13:38:02Z

demos/open-shift-install/java-client-installer.sh

+
+usage()
+{
+    echo "usage: installer [[[--ocp-url url ] [--docker-url url ] [--project-name project] [--account-name account] [--authenticator authenticator]] | [-h]]"


Can we expand the --help flag output to be multi-line? Although these flags are all well named, it would help the user better understand what each flag does. An example of what I mean is here: https://github.com/conjurinc/appliance/blob/master/release#L3

codeclimate · 2020-02-09T09:34:19Z

demos/java-api-client/src/main/java/com/cyberark/example/JavaClient.java

+
+public class JavaClient {
+
+    public static void main(String args[]) {


Method main has 33 lines of code (exceeds 25 allowed). Consider refactoring.

jvanderhoof · 2020-02-11T18:33:22Z

demos/open-shift-install/README.txt

+Having github user
+Maven - Apache Maven 3.6.3
+Java SDK / JRE - openjdk version "1.8.0_232"
+MAC OS Catalina - Version 10.15.1 (19076)


Is this really a hard requirement? Can we reduce these requirements by moving the libraries into containers?

we could eliminate these requirements if we would use docker - but then docker would become mandatory on inseparable element of this demo

jvanderhoof · 2020-02-11T18:39:50Z

@oburstein-hub, although I'd like to see us go a bit further, I think there is a ton of value in this PR. I'm comfortable getting this merged.

If you do merge, please use a squash merge. The Git comments don't convey much in the way of a story.

rafis3

@oburstein-hub I haven't finished the review yet, but I would love for you to take a look at what I commented so far. I hope I'll be able to finish it tomorrow.

rafis3 · 2020-02-11T14:33:33Z

demos/open-shift-install/installer.sh

+function install {
+
+  oc login $OPENSHIFT_URL
+  oc adm prune images


Isn't this command dangerous? Since you don't prune specific images, can't it accidentally delete images that are unrelated to this demo?

oc adm prune images deletes only unreferenced images. It not it safe enough ?

How do you know that your user wants to delete unreferenced images? And what benefit does the command give to your script?

rafis3 · 2020-02-11T14:38:07Z

demos/open-shift-install/installer.sh

+    oc delete ClusterRoleBinding conjur-oss-conjur-authenticator
+  } &> /dev/null
+
+  ACTUAL_DATA_KEY=$( echo "${DATA_KEY/\//\\\/}" )


Why do we need this variable? Couldn't we create the actual data key above, and set DATA_KEY with this value?

this code does not appear any more - you looked on old version

rafis3 · 2020-02-11T14:38:19Z

demos/open-shift-install/templates/custom-values.yaml

@@ -0,0 +1,15 @@
+authenticators: "authn-k8s/AUTHENTICATOR,authn"
+dataKey: "DATA_KEY"


Since this is our standards in other repos, would you mind changing the syntax of placeholders to: {{ NAME }}

rafis3 · 2020-02-11T14:45:54Z

demos/java-api-client/build.sh

@@ -0,0 +1,68 @@
+#!/bin/bash


If you have commands that you don't mind failing, I think you should prevent an error message from appearing or add a message that explains that it's ok. Otherwise, the user won't have high confidence that the run was successful. With oc commands, you can add the flag --ignore-not-found to prevent these errors.

rafis3 · 2020-02-11T14:49:26Z

demos/java-api-client/build.sh

+
+rm -rf target
+
+echo "Cloning Conjur Java SDK repository from GIT"


Suggested change

echo "Cloning Conjur Java SDK repository from GIT"

echo "Cloning Conjur Java SDK repository from GitHub"

rafis3 · 2020-02-12T00:29:32Z

demos/open-shift-install/README.txt

+The purpose of this demo is to install Conjur on existing OpenShift environment and then run Java Client on top of it
+The environent contains 4 pods each with up to 2 containers inside
+Pod #1: Postgres
+Pod #2: Conjur + Ngnx


Suggested change

Pod #2: Conjur + Ngnx

Pod #2: Conjur + Nginx

rafis3 · 2020-02-12T00:30:03Z

demos/open-shift-install/README.txt

+Pod #1: Postgres
+Pod #2: Conjur + Ngnx
+Pod #3: Conjur CLI
+Pod #4: Authenticator + Java Client 


Suggested change

Pod #4: Authenticator + Java Client

Pod #4: Conjur authenticator client + Java client

rafis3 · 2020-02-12T00:32:28Z

demos/open-shift-install/README.txt

+Prerequisites:
+--------------
+Git - git version 2.24.1
+Having github user


Suggested change

Having github user

A GitHub user

rafis3 · 2020-02-12T00:33:13Z

demos/open-shift-install/README.txt

+Having github user
+Maven - Apache Maven 3.6.3
+Java SDK / JRE - openjdk version "1.8.0_232"
+MAC OS Catalina - Version 10.15.1 (19076)


Why do you need macOS Catalina? Will it not work with Linux or older macOS versions? Same with other prerequisites, we will provide less value if we will tell our users that they must have these exact versions.

I wanted to mention on which system it was actually tried
Do you still want me to remove it ?

I don't believe we need there's any reason to be so specific to the OSX version.

rafis3 · 2020-02-12T00:35:35Z

demos/open-shift-install/README.txt

+OpenShift - 	oc v3.11.0+0cbc58b
+		kubernetes v1.11.0+d4cacc0
+		features: Basic-Auth
+OpenShift client installed on MAC


General comment:
What do you think about splitting the prerequisites between what you need to have locally (tools, OS version and so on) and the external prerequisites like the OpenShift environment, user and so on? I think it would be clearer

rafis3 · 2020-02-12T09:37:45Z

demos/open-shift-install/README.txt

+Pod #3: Conjur CLI
+Pod #4: Authenticator + Java Client 
+
+Prerequisites:


It would be really great to turn the README from plaintext to a markdown page. It will look much clearer.

done - according to definitions of that I have found in google

rafis3 · 2020-02-12T13:07:21Z

demos/java-api-client/README.txt

+Instructions for building java client
+-------------------------------------
+
+For compiling java test application please run: ./build.sh  


Following my previous comment on a markdown format, see this for more information on how you format titles, code, bullets and so on: https://www.markdownguide.org/basic-syntax/

In addition, once you change the file type to md instead of txt, GitHub will show it in a readable format.

I have used the same manual you sent - The Alternative Syntax part of it

README files changed from txt to md

rafis3 · 2020-02-12T13:12:00Z

demos/open-shift-install/cleanup.sh

+#set -e
+
+function validate_app {
+        APPNAME=$1


Can you please replaces the indention to 2 spaces? That's how most of our scripts are written, I find it easier to read.

rafis3 · 2020-02-12T13:13:02Z

demos/open-shift-install/cleanup.sh

+
+oc delete deployment --all
+helm uninstall conjur-oss
+oc delete project $1


If you delete the project, it deletes all the resources inside it. So you don't need to delete the deployments above, since this line already does it for you.

rafis3 · 2020-02-12T13:15:42Z

demos/open-shift-install/installer.sh

@@ -0,0 +1,204 @@
+#!/bin/bash
+


Sorry for this small comment, but can you please rename the folder from open-shift to openshift? This is how it's written according to Red Hat.

rafis3

@oburstein-hub added some additional comments. Except these, I think the only thing left for me to review is the README file, which I will do once you turn it into markdown.

rafis3 · 2020-02-12T13:41:51Z

demos/open-shift-install/installer.sh

+  oc get pods
+
+  if [ "$CONTAINERS_STATUS" != "2/2" ]; then
+    echo "Conjur POD did not come up properly - exiting"


Suggested change

echo "Conjur POD did not come up properly - exiting"

echo "Conjur pod did not start properly - exiting"

rafis3 · 2020-02-12T13:45:34Z

demos/open-shift-install/java-client-installer.sh

+  if [ "$CONTAINERS_STATUS" == "2/2" ]; then
+    break
+  fi
+  echo "Waiting for java client pod to be up..."


Suggested change

echo "Waiting for java client pod to be up..."

echo "Waiting for the Java client pod to start..."

rafis3 · 2020-02-12T13:47:08Z

demos/open-shift-install/templates/conjur-java-api-example.yaml

+            name: conjur-access-token
+      - name: my-conjur-java-client
+        image: docker-registry.default.svc:5000/{{ PROJECT_NAME }}/conjur-java-client:latest
+        imagePullPolicy: Always


I recommend that this and the other one to be switched from Always to IfNotPresent

rafis3 · 2020-02-12T13:49:30Z

demos/open-shift-install/templates/conjur-java-api-example.yaml

+          - name: CONJUR_ACCOUNT
+            value: {{ ACCOUNT_NAME }}
+          - name: CONJUR_AUTHN_LOGIN
+            value: admin


Why is this using the admin user? Shouldn't this be using the host you created?

what host ?

CONJUR_AUTHN_LOGIN property is required for Conjur Java SDK. Its description is provided in SDK Readme:
CONJUR_AUTHN_LOGIN - user/host identity.

rafis3 · 2020-02-12T13:50:37Z

demos/java-api-client/pom.xml

+                        <CONJUR_ACCOUNT>${env.CONJUR_ACCOUNT}</CONJUR_ACCOUNT>
+                        <CONJUR_APPLIANCE_URL>${env.CONJUR_APPLIANCE_URL}</CONJUR_APPLIANCE_URL>
+                        <CONJUR_AUTHN_LOGIN>${env.CONJUR_AUTHN_LOGIN}</CONJUR_AUTHN_LOGIN>
+                        <CONJUR_AUTHN_API_KEY>${env.CONJUR_AUTHN_API_KEY}</CONJUR_AUTHN_API_KEY>


Why does the Java app need the admin API key? It is supposed to connect with the host and go through the Kubernetes authentication process, right?

rafis3 · 2020-02-12T13:53:46Z

demos/open-shift-install/installer.sh

+
+  echo "Load conjur policy"
+
+  oc rsync policy "$CONJUR_CLI_POD":/


Have you considered adding these into the container as a ConfigMap? This way, your app doesn't have to wait for the file to appear, it will be mounted from the beginning. See more details here: https://docs.openshift.com/container-platform/3.11/dev_guide/configmaps.html

I don't wait to the policy files to appear anyway - they are copied before conjur init

rafis3 · 2020-02-12T13:59:43Z

demos/open-shift-install/templates/conjur-java-api-example.yaml

+    spec:
+      serviceAccountName: default
+      containers:
+      - image: cyberark/conjur-authn-k8s-client:latest


Please use this image instead: cyberark/conjur-kubernetes-authenticator
It should work the same. We had a duplication which we have just recently deprecated.

jvanderhoof · 2020-02-12T16:28:47Z

demos/java-api-client/build.sh

+  exit 1
+fi
+
+BRANCH_NAME=$( git branch | grep "*" | awk '{print $2}' )


alternatively, you could use git rev-parse --abbrev-ref HEAD, which provides the current branch (which should be master).

codeclimate · 2020-02-17T13:23:09Z

Code Climate has analyzed commit 5c40b8d and detected 0 issues on this pull request.

View more on Code Climate.

rafis3

Looks good to me, please merge.
Thanks @oburstein-hub

oburstein-hub · 2020-02-20T09:13:05Z

Thanks a lot! From: Rafi Schwarz <[email protected]> Reply-To: conjurdemos/conjur-intro <[email protected]> Date: Thursday, 20 February 2020 at 11:12 To: conjurdemos/conjur-intro <[email protected]> Cc: Ofira Burstein <[email protected]>, Mention <[email protected]> Subject: Re: [conjurdemos/conjur-intro] adding Java SDK client example and scripts to run it on openshift (#46) @rafis3 approved this pull request. Looks good to me, please merge. Thanks @oburstein-hub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_oburstein-2Dhub&d=DwMCaQ&c=E55fojPA83XrPGfndbiaQQ&r=JfJsG0whNR0XwkLaCxjWy_WXYgUA5ifZ79MYKc9m6Qg&m=gExaik80mr4zuSyEBJJuNPG2F-pJ5J7MQl8Hn-8BlqA&s=XA1XviThFb2cvJjfg95BhN6jIR3_6R0CzyUuMj_GErU&e=> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_conjurdemos_conjur-2Dintro_pull_46-3Femail-5Fsource-3Dnotifications-26email-5Ftoken-3DAOFI64SDSTUVE2LVDE2NR3TRDZCP3A5CNFSM4KQLRLCKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWH2OBY-23pullrequestreview-2D361735943&d=DwMCaQ&c=E55fojPA83XrPGfndbiaQQ&r=JfJsG0whNR0XwkLaCxjWy_WXYgUA5ifZ79MYKc9m6Qg&m=gExaik80mr4zuSyEBJJuNPG2F-pJ5J7MQl8Hn-8BlqA&s=y3XHi5IgjGx_rU4s1XsqkexxKEZp4Z0ZsTcZFC5kd-A&e=>, or unsubscribe<https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AOFI64QNX7UL2MTHG4Z5SGTRDZCP3ANCNFSM4KQLRLCA&d=DwMCaQ&c=E55fojPA83XrPGfndbiaQQ&r=JfJsG0whNR0XwkLaCxjWy_WXYgUA5ifZ79MYKc9m6Qg&m=gExaik80mr4zuSyEBJJuNPG2F-pJ5J7MQl8Hn-8BlqA&s=295Jb9k1Pu4nxIE17T-62yQ8ngsec6Z4SQwtHlFiCng&e=>.

…

----------------------------------------------------------------------

_______________________________________________ This e-mail may contain information that is confidential, privileged or otherwise protected from disclosure. If you are not an intended recipient of this e-mail, do not duplicate or redistribute it by any means. Please delete it and any attachments and notify the sender that you have received it in error.

Change pg version to 10.16

oburstein-hub requested review from rafis3 and jvanderhoof February 5, 2020 13:59

jvanderhoof reviewed Feb 6, 2020

View reviewed changes

jvanderhoof reviewed Feb 7, 2020

View reviewed changes

codeclimate bot reviewed Feb 9, 2020

View reviewed changes

jvanderhoof reviewed Feb 11, 2020

View reviewed changes

rafis3 requested changes Feb 12, 2020

View reviewed changes

rafis3 reviewed Feb 12, 2020

View reviewed changes

oburstein-hub force-pushed the OCP_installer_and_java_test branch 2 times, most recently from 79743b2 to 6759ff0 Compare February 12, 2020 12:15

rafis3 reviewed Feb 12, 2020

View reviewed changes

rafis3 requested changes Feb 12, 2020

View reviewed changes

oburstein-hub force-pushed the OCP_installer_and_java_test branch from adedb3e to ca0d27e Compare February 12, 2020 15:20

jvanderhoof reviewed Feb 12, 2020

View reviewed changes

oburstein-hub force-pushed the OCP_installer_and_java_test branch from 202e2f4 to 0d7e9f9 Compare February 13, 2020 14:04

adding Java SDK client example and scripts to run it on openshift

5c40b8d

oburstein-hub force-pushed the OCP_installer_and_java_test branch from 0d7e9f9 to 5c40b8d Compare February 17, 2020 13:22

rafis3 approved these changes Feb 20, 2020

View reviewed changes

oburstein-hub merged commit 595525a into master Feb 20, 2020

jvanderhoof pushed a commit that referenced this pull request Sep 9, 2022

Merge pull request #46 from cyberark/izgeri-patch-1

1bf9ff1

Change pg version to 10.16


		public class JavaClient {

		public static void main(String args[]) {

		@@ -0,0 +1,15 @@
		authenticators: "authn-k8s/AUTHENTICATOR,authn"
		dataKey: "DATA_KEY"


		rm -rf target

		echo "Cloning Conjur Java SDK repository from GIT"

	Pod #4: Authenticator + Java Client
	Pod #4: Conjur authenticator client + Java client

	echo "Conjur POD did not come up properly - exiting"
	echo "Conjur pod did not start properly - exiting"

	echo "Waiting for java client pod to be up..."
	echo "Waiting for the Java client pod to start..."


		echo "Load conjur policy"

		oc rsync policy "$CONJUR_CLI_POD":/

adding Java SDK client example and scripts to run it on openshift #46

adding Java SDK client example and scripts to run it on openshift #46

Conversation

oburstein-hub commented Feb 5, 2020

What does this PR do? Is there any background context you want to provide?

What ticket does this PR close?

Where should the reviewer start? How should this functionality be validated?

Screenshots (if appropriate)

Links to open issues for related documentation (in READMEs, docs, etc)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot Feb 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oburstein-hub Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

jvanderhoof commented Feb 11, 2020

rafis3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oburstein-hub Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafis3 Feb 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafis3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oburstein-hub Feb 12, 2020 •

edited

Loading

oburstein-hub Feb 12, 2020 •

edited

Loading

rafis3 Feb 12, 2020 •

edited

Loading